fix(credential): forward [id] positional as credential id via PUT#48
Merged
shreemaan-abhishek merged 3 commits intoMay 26, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe PR forwards the optional positional ChangesCredential Create: Positional ID Forwarding and Display Name Flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/credential/create/create_test.go (1)
36-120: ⚡ Quick winAdd YAML output coverage for the new shared submit path.
The new
submitoutput branch is exercised only with JSON assertions right now. Please add at least one-o yamlcase (POST or PUT) to cover the formatting path introduced in this PR.As per coding guidelines:
**/*.go: “Write tests for every code change”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cmd/credential/create/create_test.go` around lines 36 - 120, Add a new unit test (e.g., TestCreateCredential_YAMLOutput) mirroring TestCreateCredential_JSONOutput but exercising the shared submit path with YAML formatting: create an httpmock.Registry responder for the same POST/PUT used in the other tests, build an Options struct like in the existing tests but set the Options output field to request YAML (set the Options.Output or Options.OutputFormat field to "yaml" to mirror how the CLI exposes -o yaml), call actionRun(opts), unmarshal/parse out.Bytes() as YAML and assert key fields (id, name/desc) match the mock response, and call registry.Verify(t). Ensure you reuse Options fields such as IO, Client, Config, Consumer, GatewayGroup, PluginsJSON and reference actionRun and Options to find where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/credential/create/create.go`:
- Around line 149-153: In submit(), consumer (opts.Consumer) and id are being
interpolated directly into the path and ggID into the query—escape them first to
prevent path/query injection: use url.PathEscape for path segments (consumer and
id used in the path in the fmt.Sprintf inside submit) and url.QueryEscape for
ggID used in the ?gateway_group_id= query string, then build the fmt.Sprintf
with the escaped values before calling client.Put or client.Post; update the two
call sites in submit() accordingly.
---
Nitpick comments:
In `@pkg/cmd/credential/create/create_test.go`:
- Around line 36-120: Add a new unit test (e.g.,
TestCreateCredential_YAMLOutput) mirroring TestCreateCredential_JSONOutput but
exercising the shared submit path with YAML formatting: create an
httpmock.Registry responder for the same POST/PUT used in the other tests, build
an Options struct like in the existing tests but set the Options output field to
request YAML (set the Options.Output or Options.OutputFormat field to "yaml" to
mirror how the CLI exposes -o yaml), call actionRun(opts), unmarshal/parse
out.Bytes() as YAML and assert key fields (id, name/desc) match the mock
response, and call registry.Verify(t). Ensure you reuse Options fields such as
IO, Client, Config, Consumer, GatewayGroup, PluginsJSON and reference actionRun
and Options to find where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59f5d968-8ca7-4a9a-abbf-74950f0bbca9
📒 Files selected for processing (4)
docs/ga-test-report.mddocs/user-guide/credential.mdpkg/cmd/credential/create/create.gopkg/cmd/credential/create/create_test.go
API7 EE control-plane upserts on PUT /apisix/admin/consumers/<u>/credentials/<id>
(middleware: interceptor.go:386-400 builds the credential from the body
when GetCredential returns NotFound and the path matches; DAO uses
OnConflict{UpdateAll} keyed on (gateway_group_id, credential_id); the
BeforeCreate GORM hook only auto-generates a UUID when the client did
not supply one). So we can honor the positional id end-to-end.
Before this change, `a7 credential create my-id ...` POSTed with the
positional placed in `name` and returned a server-assigned UUID,
contradicting the help text and breaking scripts that expect their
chosen id back.
After:
- positional `[id]` and `id:` in `--file` payloads are forwarded as the
URL path on PUT. Positional wins on conflict.
- Server assigns the id when the positional is omitted (POST path
unchanged).
- New `--name` flag carries the display name, no longer conflated with
id.
- Tests cover: PUT-with-positional, PUT-with-file-id,
positional-overrides-file, --name on POST, and invalid-id rejection.
Docs and the GA test report updated to reflect the new contract.
Closes #36.
API7 EE rejects credentials whose `name` is empty (openapi ConsumerCredential.required includes name; common.yaml Name is minLength: 1). After the previous commit the CLI started forwarding positional [id] as the URL path on PUT but stopped putting anything in the `name` field, so the e2e suite hit 400 Bad Request on `a7 credential create <id> --consumer ... --plugins-json ...`. Mirror the id into `name` when the caller did not pass --name (or did not supply `name:` in a --file payload). This matches the pre-fix ergonomics for callers who treated the positional as both id and name. Explicit --name still wins. Adds two unit tests for the auto-derive behavior on both code paths.
shreemaan-abhishek
force-pushed
the
fix/credential-create-forward-id-via-put
branch
from
cbd4e7e to
e97d665
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a7 credential create [id] so that a user-supplied credential id is honored by sending an upsert PUT /apisix/admin/consumers/<user>/credentials/<id> (instead of POST + server-generated UUID), aligning CLI behavior with the help text and closing #36. It also introduces a dedicated --name flag to set the credential display name independently of the id.
Changes:
- Forward positional
[id](andid:from--file) as the credential id viaPUT, falling back toPOSTwhen omitted. - Add
--nameflag and adjust create-time name derivation (mirror id into name when required and--nameis absent). - Update unit tests and documentation to reflect new id/name semantics and precedence rules.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cmd/credential/create/create.go | Implements PUT-vs-POST submission based on provided id; adds --name; refactors request submission flow. |
| pkg/cmd/credential/create/create_test.go | Updates and expands tests to cover id forwarding via PUT, precedence, and invalid id handling. |
| docs/user-guide/credential.md | Documents new [id] behavior, --name, and updated examples. |
| docs/ga-test-report.md | Updates GA test report notes to reflect the corrected positional id behavior and new --name flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
86
to
95
| id := opts.ID | ||
| if id == "" { | ||
| if raw, ok := payload["id"]; ok { | ||
| idStr, ok := raw.(string) | ||
| if !ok || strings.TrimSpace(idStr) == "" { | ||
| return fmt.Errorf("credential id must be a non-empty string") | ||
| } | ||
| id = idStr | ||
| } | ||
| } |
| // gave us an id but no explicit name, mirror the id into the name. | ||
| if _, hasName := payload["name"]; !hasName && id != "" { | ||
| payload["name"] = id | ||
| } |
| return "", false, fmt.Errorf("credential name must be a non-empty string") | ||
| } | ||
| return name, true, nil | ||
| return cmdutil.NewExporter(format, opts.IO.Out).WriteAPIResponse(raw) |
Addresses three review comments on #48: - Escape `opts.Consumer`, `id`, and `ggID` before interpolating them into the credential URL. Without escaping, special characters (`/`, `?`, `&`, `%`) could re-route the request or corrupt the query string. - Validate `payload["id"]` from a `--file` payload whenever it is present, not only when the positional override is empty. A bogus value (non-string or whitespace-only) is now surfaced even when the positional supplies the URL segment. - Validate `payload["name"]` (after `--name` override and id->name defaulting) so a non-string or whitespace-only name from `--file` errors out client-side with a clear message instead of falling through to a less specific server-side error. - Normalize positional and file id/name with `strings.TrimSpace` so `id: " foo "` produces `PUT /credentials/foo`, not the original whitespace. The `stringField` helper consolidates the present/string/non-empty check and writes the trimmed value back to the payload. Tests cover URL escaping (consumer with `/`, id with space, gateway group with `&`), file id rejection when the positional overrides, trimming of whitespace in the file id, and rejection of a non-string name in the file payload.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cmd/credential/create/create.go`:
- Around line 139-146: The current logic trims opts.Name into name and silently
allows a whitespace-only --name to become empty when no id is supplied; change
the validation so that if opts.Name is non-empty (the user passed --name) but
strings.TrimSpace(opts.Name) yields an empty string and we're not in the file
flow (i.e., id == ""), the command returns an error instead of proceeding.
Locate the block that sets id := strings.TrimSpace(opts.ID) and name :=
strings.TrimSpace(opts.Name) (and constructs api.Credential{Name: name, Desc:
opts.Desc}) and add a guard that checks opts.Name != "" && name == "" && id ==
"" and returns a user-facing error asking for a non-empty name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78c5d5df-a454-4905-96cf-fec3dfc22a02
📒 Files selected for processing (4)
docs/ga-test-report.mddocs/user-guide/credential.mdpkg/cmd/credential/create/create.gopkg/cmd/credential/create/create_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/ga-test-report.md
Comment on lines
+139
to
+146
| id := strings.TrimSpace(opts.ID) | ||
| name := strings.TrimSpace(opts.Name) | ||
| if name == "" && id != "" { | ||
| // API7 EE requires a non-empty `name`. Mirror the id when the caller | ||
| // did not pass --name explicitly. | ||
| name = id | ||
| } | ||
| bodyReq := api.Credential{Name: name, Desc: opts.Desc} |
There was a problem hiding this comment.
Reject whitespace-only --name in non-file flow.
Line 140 trims --name, but if a user passes whitespace-only input and no positional id, the request proceeds with an empty name instead of failing fast (file flow rejects this case).
Suggested patch
id := strings.TrimSpace(opts.ID)
name := strings.TrimSpace(opts.Name)
+if opts.Name != "" && name == "" {
+ return fmt.Errorf("credential name must be a non-empty string")
+}
if name == "" && id != "" {
// API7 EE requires a non-empty `name`. Mirror the id when the caller
// did not pass --name explicitly.
name = id
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| id := strings.TrimSpace(opts.ID) | |
| name := strings.TrimSpace(opts.Name) | |
| if name == "" && id != "" { | |
| // API7 EE requires a non-empty `name`. Mirror the id when the caller | |
| // did not pass --name explicitly. | |
| name = id | |
| } | |
| bodyReq := api.Credential{Name: name, Desc: opts.Desc} | |
| id := strings.TrimSpace(opts.ID) | |
| name := strings.TrimSpace(opts.Name) | |
| if opts.Name != "" && name == "" { | |
| return fmt.Errorf("credential name must be a non-empty string") | |
| } | |
| if name == "" && id != "" { | |
| // API7 EE requires a non-empty `name`. Mirror the id when the caller | |
| // did not pass --name explicitly. | |
| name = id | |
| } | |
| bodyReq := api.Credential{Name: name, Desc: opts.Desc} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/cmd/credential/create/create.go` around lines 139 - 146, The current
logic trims opts.Name into name and silently allows a whitespace-only --name to
become empty when no id is supplied; change the validation so that if opts.Name
is non-empty (the user passed --name) but strings.TrimSpace(opts.Name) yields an
empty string and we're not in the file flow (i.e., id == ""), the command
returns an error instead of proceeding. Locate the block that sets id :=
strings.TrimSpace(opts.ID) and name := strings.TrimSpace(opts.Name) (and
constructs api.Credential{Name: name, Desc: opts.Desc}) and add a guard that
checks opts.Name != "" && name == "" && id == "" and returns a user-facing error
asking for a non-empty name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[id]; closed in favor of this).a7 credential create <id> ...previously POSTed and the server returned a fresh UUID, contradicting the help text. After auditing api7-ee-3 control-plane, PUT/apisix/admin/consumers/<u>/credentials/<id>is an upsert that honors a client-supplied id (interceptor.go:386-400; DAOOnConflict{UpdateAll};BeforeCreateonly generates a UUID whenCredentialIDis empty).[id](andid:inside a--filepayload) as the URL path on PUT. When omitted, POST + server-assigned UUID is unchanged.--nameflag carries the display name, no longer conflated with id.Precedence
[id]overridespayload["id"]from--file(kubectl/gh convention).id:in a file payload is rejected client-side with a clear error.Test plan
go test ./pkg/cmd/credential/...— pass. New/updated cases:TestCreateCredential_PositionalForwardsAsIDViaPUTTestCreateCredential_NameFlagPOSTsTestCreateCredential_FileIDForwardedViaPUTTestCreateCredential_PositionalOverridesFileIDTestCreateCredential_FileRejectsInvalidIDgo test ./...— pass.go build ./...— clean.a7 credential create my-key --consumer alice -g default --plugins-json '{"key-auth":{"key":"k"}}'— returnsid: my-key.a7 credential create --consumer alice -g default --name "Jack key" --plugins-json '...'— returns server-assigned UUID, name set.a7 credential create -f cred.yaml --consumer alice -g defaultwherecred.yamlhasid: from-file— credential created atfrom-file.Docs (
docs/user-guide/credential.md) anddocs/ga-test-report.mdupdated.Note:
make lintsurfaces a pre-existingyamltypecheck error inpkg/cmdutilandpkg/cmd/config/configutil— reproducible on plain master, unrelated to this change.Summary by CodeRabbit
New Features
--nameflag to set credential display name on creation.Bug Fixes
Documentation
Tests